fix(sidebar): prevent null responses from sidebar API endpoints#974
fix(sidebar): prevent null responses from sidebar API endpoints#974DHRUVI5674 wants to merge 1 commit intomattermost:masterfrom
Conversation
… returned instead of nil
|
Hello @DHRUVI5674, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project. Please help complete the Mattermost contribution license agreement? This is a standard procedure for many open source projects. Please let us know if you have any questions. We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe change modifies error handling in the API layer to gracefully degrade on failures from GitHub by returning empty data structures instead of null or propagating errors, ensuring the webapp remains functional during temporary outages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/plugin/api.go (1)
1045-1064: Consider: swallowing errors prevents the frontend from showing error state.Per the relevant code snippets, the webapp's
getSidebarContentaction usesisAPIError(data)to distinguish between error responses and successful data. With this change, the backend always returns HTTP 200 with valid JSON, soisAPIErrorwill never be true for sidebar content.This means:
- Users cannot distinguish between "GitHub API is down" vs "you have zero PRs/reviews"
- The linked issue (
#235) mentions the backend should "return a non-200 status if fetching from GitHub fails"If this behaviour is intentional for resilience (as stated in PR objectives), consider at minimum adding a boolean field like
partial: trueorfetchError: trueto the response so the frontend can optionally display a warning indicator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/plugin/api.go`:
- Around line 1068-1077: The build fails because logger.Logger has no Warn
method; update the defensive logging in the block that checks "if err != nil ||
sidebarContent == nil" to call c.Log.Warnf instead of c.Log.Warn, e.g., use
c.Log.Warnf("Sidebar content empty, returning defaults") so the call matches the
logger API; leave the nil-check and defaulting for sidebarContent (and note
getSidebarData still returns non-nil in current code) as a safeguard.
- Line 1048: Replace the non-existent call to c.Log.WithError(err).Warn by using
the existing Warnf pattern: call c.Log.WithError(err).Warnf with a formatted
message string (e.g., "Sidebar data fetch failed: %v") and pass the error or
relevant args so the logger compiles and mirrors other calls in this file (see
other uses of c.Log.WithError(...).Warnf).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c1db2c0-eddf-42e4-9e43-f2b4efc06bcd
📒 Files selected for processing (1)
server/plugin/api.go
| if err != nil { | ||
| return nil, err | ||
| } | ||
| c.Log.WithError(err).Warn("Sidebar data fetch failed") |
There was a problem hiding this comment.
Build failure: Warn method does not exist on logger.Logger.
The static analysis indicates that logger.Logger has no method Warn. Use Warnf instead, which is consistent with other logging calls in this file (e.g., line 721).
🐛 Proposed fix
- c.Log.WithError(err).Warn("Sidebar data fetch failed")
+ c.Log.WithError(err).Warnf("Sidebar data fetch failed")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c.Log.WithError(err).Warn("Sidebar data fetch failed") | |
| c.Log.WithError(err).Warnf("Sidebar data fetch failed") |
🧰 Tools
🪛 GitHub Check: plugin-ci / test
[failure] 1048-1048:
c.Log.WithError(err).Warn undefined (type logger.Logger has no field or method Warn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/plugin/api.go` at line 1048, Replace the non-existent call to
c.Log.WithError(err).Warn by using the existing Warnf pattern: call
c.Log.WithError(err).Warnf with a formatted message string (e.g., "Sidebar data
fetch failed: %v") and pass the error or relevant args so the logger compiles
and mirrors other calls in this file (see other uses of
c.Log.WithError(...).Warnf).
| if err != nil || sidebarContent == nil { | ||
| c.Log.Warn("Sidebar content empty, returning defaults") | ||
|
|
||
| sidebarContent = &SidebarContent{ | ||
| PRs: []*graphql.GithubPRDetails{}, | ||
| Reviews: []*graphql.GithubPRDetails{}, | ||
| Assignments: []*github.Issue{}, | ||
| Unreads: []*FilteredNotification{}, | ||
| } | ||
| } |
There was a problem hiding this comment.
Build failure: Warn method does not exist on logger.Logger.
Same issue as above - use Warnf instead of Warn.
Additionally, note that this defensive block is now effectively unreachable: getSidebarData always returns a non-nil SidebarContent and nil error after your changes. The check is still reasonable as a safeguard, but consider whether this logging path will ever execute.
🐛 Proposed fix
if err != nil || sidebarContent == nil {
- c.Log.Warn("Sidebar content empty, returning defaults")
+ c.Log.Warnf("Sidebar content empty, returning defaults")
sidebarContent = &SidebarContent{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil || sidebarContent == nil { | |
| c.Log.Warn("Sidebar content empty, returning defaults") | |
| sidebarContent = &SidebarContent{ | |
| PRs: []*graphql.GithubPRDetails{}, | |
| Reviews: []*graphql.GithubPRDetails{}, | |
| Assignments: []*github.Issue{}, | |
| Unreads: []*FilteredNotification{}, | |
| } | |
| } | |
| if err != nil || sidebarContent == nil { | |
| c.Log.Warnf("Sidebar content empty, returning defaults") | |
| sidebarContent = &SidebarContent{ | |
| PRs: []*graphql.GithubPRDetails{}, | |
| Reviews: []*graphql.GithubPRDetails{}, | |
| Assignments: []*github.Issue{}, | |
| Unreads: []*FilteredNotification{}, | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: plugin-ci / test
[failure] 1069-1069:
c.Log.Warn undefined (type logger.Logger has no field or method Warn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/plugin/api.go` around lines 1068 - 1077, The build fails because
logger.Logger has no Warn method; update the defensive logging in the block that
checks "if err != nil || sidebarContent == nil" to call c.Log.Warnf instead of
c.Log.Warn, e.g., use c.Log.Warnf("Sidebar content empty, returning defaults")
so the call matches the logger API; leave the nil-check and defaulting for
sidebarContent (and note getSidebarData still returns non-nil in current code)
as a safeguard.
marianunez
left a comment
marianunez
left a comment
There was a problem hiding this comment.
@DHRUVI5674 thank you for your contribution!
Please take a look at the lint failures and also sign the CLA.
3 participants
Fixes #235
Summary
Sidebar API endpoints sometimes returned
nullwhen GitHub API calls failed or returned unexpected data. This caused the Mattermost webapp sidebar to crash.Changes
nilingetUnreadsDatagetSidebarDatareturns defaultSidebarContenton errorTesting
Result
The sidebar no longer crashes when API endpoints return null or fail unexpectedly.
Type of change
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor